Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.39%. Comparing base (bedfdd6) to head (063f85b).
⚠️ Report is 6 commits behind head on dev.

@@           Coverage Diff           @@
##              dev     #483   +/-   ##
=======================================
  Coverage   71.39%   71.39%           
=======================================
  Files        1219     1219           
  Lines       88382    88382           
  Branches    11565    11565           
=======================================
+ Hits        63101    63104    +3     
+ Misses      20748    20746    -2     
+ Partials     4533     4532    -1     

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes

Review Summary

Overall this is a solid PR that cleanly fixes both #476 and #482 by replacing raw Lark JSON card blobs with structured MessageContent objects. The refactoring is well-targeted, tests are thorough, and dead code removal is appropriate.

I found a few issues worth addressing:

1. AgentBuilderCardContent duplicates constants already defined in AgentBuilderCardFlow

AgentBuilderCardContent.cs re-declares these constants:

• DailyReportAction = "create_daily_report"
• SocialMediaAction = "create_social_media"
• OpenDailyReportFormAction = "open_daily_report_form"
• OpenSocialMediaFormAction = "open_social_media_form"
• ListTemplatesAction = "list_templates"
• ListAgentsAction = "list_agents"
• DefaultScheduleTime = "09:00"

These are identical to the ones in AgentBuilderCardFlow.cs lines 15-27. The two classes are tightly coupled — AgentBuilderCardFlow.FormatToolResult calls AgentBuilderCardContent.FormatListAgentsResult, and AgentBuilderCardFlow.TryResolve calls AgentBuilderCardContent.BuildDailyReportForm. Diverging constant values would be a silent bug. Consider moving shared constants into AgentBuilderCardContent (or a dedicated constants class) and having AgentBuilderCardFlow reference them.

Severity: Medium — risk of silent divergence.

2. Triplicated JSON helper methods

AgentBuilderCardContent has its own TryReadError, TryReadString, TryReadBool, TryReadOptional, and TextContent helpers that are functionally identical (or near-identical) to the ones in AgentBuilderCardFlow.cs and NyxRelayAgentBuilderFlow.cs. Bug fixes to one copy need replicating to the others. Consider extracting shared JSON parsing helpers into a small internal utility class (e.g. AgentBuilderJson).

Severity: Medium — maintenance hazard.

3. LarkMessageComposer skipTitle = i == 0 silently drops titles in edge cases

LarkMessageComposer.cs:113 unconditionally skips the first card's title from body markdown. However, ResolveHeaderTitle returns effectiveText ?? "Aevatar" — if MessageContent.Text is non-empty, the header title comes from Text, not from the first card's Title. In that scenario the first card's Title is dropped from the rendered body for no reason.

Doesn't trigger with current callers but deserves a guard or at least a comment.

Severity: Low — latent, not triggered today.

4. Nyx relay path still returns plain-text for most actions

NyxRelayAgentBuilderFlow.FormatToolResult (lines 64-75) still returns TextContent(...) for create_social_media, list_templates, run_agent, disable_agent, enable_agent, delete_agent. Meanwhile AgentBuilderCardFlow now returns structured MessageContent with Cards + Actions for all these. The PR body implies both paths render the same way, but they don't for most actions beyond list_agents. Worth confirming this is intentional.

Severity: Low — UX consistency question.

5. Growing god-class

AgentBuilderCardContent is 426 lines of public static methods spanning form builders, result formatters, list renderers, and credentials cards. Consider organizing by responsibility as a follow-up.

Severity: Low — follow-up refactoring.

Overall: LGTM with minor suggestions. Items 1-2 (constant/helper duplication) are the most actionable; the rest are low-priority follow-ups.

Fix Review: All 5 Items Resolved ✅

1. Constants duplication → AgentBuilderActionIds ✅

New file AgentBuilderActionIds.cs is the single source of truth. Both AgentBuilderCardContent and AgentBuilderCardFlow now reference AgentBuilderActionIds.* via const string assignment. Clean.

2. Triplicated JSON helpers → AgentBuilderJson ✅

New file AgentBuilderJson.cs consolidates TryReadError, TryReadString, TryReadBool, TryReadOptional, TextContent. All three consumers (AgentBuilderCardContent, AgentBuilderCardFlow, NyxRelayAgentBuilderFlow) delegate to it. The file scope is deliberately narrow — good.

3. skipTitle edge case → Comment updated ✅

The comment at LarkMessageComposer.cs:108 now explicitly documents that when the first card has no Title, ResolveHeaderTitle falls back to intent.Text and the skip is a no-op. Adequate — no code change needed for a latent case with no current trigger.

4. Bonus fix: /delete-agent <id> confirm parser ✅

The new TryResolveDeleteAgentTextCommand correctly tokenizes the confirm trailer that the shared /agents renderer prints as an inline command hint. Two new tests cover both with-trailer and without-trailer paths. Nice catch — this was a real bug hidden by the refactoring.

5. Dead FormatAgentStatusResult in NyxRelayAgentBuilderFlow removed ✅

The old text-only FormatAgentStatusResult was removed. FormatAgentStatusCard (structured) remains as the sole agent-status renderer on the relay path.

LGTM. All review items addressed, bonus fix included with tests. Ship-ready.